fix: more conservative stalled download check, better logging #884
Conversation
src/playback-watcher.js
Outdated
| }); | ||
|
|
||
| // after 5 possibly stalled appends with no reset, exclude | ||
| if (this[`${type}StalledDownloads_`] < 5) { |
There was a problem hiding this comment.
Wait until 5 stalled downloads.
There was a problem hiding this comment.
Let's use a larger value here. Would 10 be ok? Maybe 15 or even 20?
| @@ -2492,6 +2491,8 @@ export default class SegmentLoader extends videojs.EventTarget { | |||
| return; | |||
| } | |||
|
|
|||
| this.trigger('appendsdone'); | |||
There was a problem hiding this comment.
Don't report aborted appends as appends
There was a problem hiding this comment.
Do you know in what instances we will abort appends? It seems like it would only happen on dispose of the source buffers, otherwise appends will continue even if the current segment loader process is aborted.
There was a problem hiding this comment.
I think it's the request itself in this case that is aborted, rather than an actual append. I was seeing it with aggressive network throttling and changing renditions.
There was a problem hiding this comment.
Might be worth us adding a test for this case.
| @@ -2492,6 +2491,8 @@ export default class SegmentLoader extends videojs.EventTarget { | |||
| return; | |||
| } | |||
|
|
|||
| this.trigger('appendsdone'); | |||
There was a problem hiding this comment.
Do you know in what instances we will abort appends? It seems like it would only happen on dispose of the source buffers, otherwise appends will continue even if the current segment loader process is aborted.
src/media-groups.js
Outdated
| @@ -744,7 +744,7 @@ export const setupMediaGroups = (settings) => { | |||
| mediaTypes.AUDIO.onTrackChanged(); | |||
| } | |||
|
|
|||
| masterPlaylistLoader.on('mediachange', () => { | |||
| masterPlaylistLoader.on(['mediachange', 'mediachanging'], () => { | |||
There was a problem hiding this comment.
Should we worry about this causing double aborts in some cases?
There was a problem hiding this comment.
After some testing, this will cause a second abort on mediachange but the segmentLoader was already in an aborted state and nothing bad happens. After some more thought though I wonder if we should stopLoaders and resync/reset on mediachanging and then startLoaders on mediachange going to see how that works
There was a problem hiding this comment.
Not sure where you're at, but an alternative is to skip this change for the PR for right now
There was a problem hiding this comment.
yeah I gave myself until the end of today to try to solve it, just about to push up a commit that takes it out and fixes the tests.
There was a problem hiding this comment.
This took so long because this change seemed to cause other race condition issues and debugging it is quite difficult. Ultimately I think we still want some kind of fix, but it an edge case so we can probably just document it and skip for now.
This was brought up because some of our users were seeing stalled download exclusions for playlists that were not stalled.